src/sdk/namespace_fs.js (1)

2128-2140: Pass the stat to avoid duplicate filesystem calls.

In both deletion paths, the code calls append_to_reclaim_wal without passing the stat parameter, which causes the method to fetch the stat again from the filesystem. However, the stat is already available in the lifecycle deletion path from the verification step.

Optimize by passing the stat when available:

At line 2128, in the lifecycle deletion path:

 try {
     files = await this._open_files(fs_context, { src_path: file_path, delete_version: true });
+    const stat = await files.delete_version.src_file.stat(fs_context);
     await this._verify_lifecycle_filter_and_unlink(fs_context, params, file_path, files.delete_version);
-    await this.append_to_reclaim_wal(fs_context, file_path);
+    await this.append_to_reclaim_wal(fs_context, file_path, stat);
 } catch (err) {

Note: The non-lifecycle path at lines 2135-2140 doesn't have easy access to the stat, so it's acceptable to let the method fetch it.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

src/sdk/namespace_fs.js (2)

1408-1409: Logic inconsistency: stat destination before file is moved there, but then log wrong path.

This code has a critical flaw flagged in the previous review that remains unaddressed:

1. You stat file_path (the destination) before the uploaded file has been moved there from upload_path
2. Errors are silently suppressed with .catch(_.noop), potentially hiding real issues
3. At line 1460, you log upload_path to the reclaim WAL, not file_path, which is inconsistent with the stat you just performed

If the intent is to detect tape reclaim on the existing destination file (before overwrite), then:

• file_path might not exist yet (ENOENT for new uploads)
• You should log file_path (the file being replaced), not upload_path
• ENOENT should be handled explicitly

Apply this diff to fix the logic:

-const file_path_stat = config.NSFS_GLACIER_DMAPI_ENABLE_TAPE_RECLAIM &&
-    await nb_native().fs.stat(fs_context, file_path).catch(_.noop);
+let file_path_stat;
+if (config.NSFS_GLACIER_DMAPI_ENABLE_TAPE_RECLAIM && !same_inode && !part_upload) {
+    try {
+        file_path_stat = await nb_native().fs.stat(fs_context, file_path);
+    } catch (err) {
+        if (err.code !== 'ENOENT') {
+            dbg.error('_finish_upload: failed to stat destination for reclaim check', file_path, err);
+        }
+        // Destination doesn't exist yet (new file), no reclaim needed
+    }
+}

And update line 1460 to use file_path:

 if (file_path_stat && file_path_stat.xattr[Glacier.GPFS_DMAPI_XATTR_TAPE_INDICATOR]) {
-    await this.append_to_reclaim_wal(fs_context, upload_path, file_path_stat);
+    await this.append_to_reclaim_wal(fs_context, file_path, file_path_stat);
 }

---

3725-3745: Add error handling to prevent reclaim logging failures from breaking main operations.

The method lacks error handling for the stat operation and WAL append, which could cause unhandled promise rejections. While debug console.log statements mentioned in the previous review appear to have been removed, the error handling issue remains critical.

Apply this diff:

 async append_to_reclaim_wal(fs_context, file_path, stat) {
     if (!config.NSFS_GLACIER_LOGS_ENABLED || !config.NSFS_GLACIER_DMAPI_ENABLE_TAPE_RECLAIM) return;

-    if (!stat) {
-        stat = await nb_native().fs.stat(fs_context, file_path);
+    try {
+        if (!stat) {
+            stat = await nb_native().fs.stat(fs_context, file_path);
+        }
+
+        const data = JSON.stringify({
+            full_path: file_path,
+            logical_size: stat.size,
+            ea: stat.xattr,
+        });
+        await NamespaceFS.reclaim_wal.append(data);
+    } catch (err) {
+        // Log error but don't fail the operation
+        dbg.error('append_to_reclaim_wal: failed to log reclaim entry', file_path, err);
     }
-
-    const data = JSON.stringify({
-        full_path: file_path,
-        logical_size: stat.size,
-        ea: stat.xattr,
-    });
-    await NamespaceFS.reclaim_wal.append(data);
 }

This ensures that reclaim logging failures don't break the main operation flow (upload, delete, etc.).

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

src/sdk/glacier.js (2)

75-84: Tighten IBMUID JSDoc phrasing.

Redundant wording; simplify for clarity.

Apply:

-    /**
-     * GPFS_DMAPI_XATTR_TAPE_UID xattr contains UID which contains the unique ID of the UID
+    /**
+     * GPFS_DMAPI_XATTR_TAPE_UID xattr contains the unique ID of the file

---

197-209: Fix typos in reclaim stub.

• JSDoc: “inindexed” → “unindexed”.
• Error text: “Unimplementented” → “Unimplemented”.

Apply:

-    /**
-     * reclaim cleans up inindexed items in the underlying
+    /**
+     * reclaim cleans up unindexed items in the underlying
@@
-    async reclaim(fs_context, log_file, failure_recorder) {
-        throw new Error('Unimplementented');
+    async reclaim(fs_context, log_file, failure_recorder) {
+        throw new Error('Unimplemented');
     }

src/sdk/namespace_fs.js (1)

3730-3750: Make append_to_reclaim_wal non-failing and filter entries.

This must never break uploads/deletes. Wrap in try/catch, and log only when DMAPI tape xattr is present. Optionally, trim xattrs (privacy).

Apply:

-    async append_to_reclaim_wal(fs_context, file_path, stat) {
-        if (!config.NSFS_GLACIER_LOGS_ENABLED || !config.NSFS_GLACIER_DMAPI_ENABLE_TAPE_RECLAIM) return;
-
-        if (!stat) {
-            stat = await nb_native().fs.stat(fs_context, file_path);
-        }
-
-        const data = JSON.stringify({
-            full_path: file_path,
-            logical_size: stat.size,
-            ea: stat.xattr,
-        });
-        await NamespaceFS.reclaim_wal.append(data);
-    }
+    async append_to_reclaim_wal(fs_context, file_path, stat) {
+        if (!config.NSFS_GLACIER_LOGS_ENABLED || !config.NSFS_GLACIER_DMAPI_ENABLE_TAPE_RECLAIM) return;
+        try {
+            if (!stat) {
+                stat = await nb_native().fs.stat(fs_context, file_path);
+            }
+            if (!stat?.xattr?.[Glacier.GPFS_DMAPI_XATTR_TAPE_INDICATOR]) return;
+            const data = JSON.stringify({
+                full_path: file_path,
+                logical_size: stat.size,
+                // keep only DMAPI-related EAs to reduce noise/sensitivity
+                ea: {
+                    [Glacier.GPFS_DMAPI_XATTR_TAPE_INDICATOR]: stat.xattr[Glacier.GPFS_DMAPI_XATTR_TAPE_INDICATOR],
+                    [Glacier.GPFS_DMAPI_XATTR_TAPE_PREMIG]: stat.xattr[Glacier.GPFS_DMAPI_XATTR_TAPE_PREMIG],
+                    [Glacier.GPFS_DMAPI_XATTR_TAPE_TPS]: stat.xattr[Glacier.GPFS_DMAPI_XATTR_TAPE_TPS],
+                    [Glacier.GPFS_DMAPI_XATTR_TAPE_UID]: stat.xattr[Glacier.GPFS_DMAPI_XATTR_TAPE_UID],
+                },
+            });
+            await NamespaceFS.reclaim_wal.append(data);
+        } catch (err) {
+            dbg.warn('append_to_reclaim_wal: failed to log reclaim entry', file_path, err);
+        }
+    }

src/sdk/namespace_fs.js (1)

2136-2145: Deletion path: WAL logging should gate on DMAPI indicator and avoid noisy warns.

• Avoid logging all deletes; check DMAPI indicator before appending.
• Wrap stat + append in try/catch; don’t warn on ENOENT.

Apply:

-            try {
-                const stat = config.NSFS_GLACIER_DMAPI_ENABLE_TAPE_RECLAIM &&
-                    await nb_native().fs.stat(fs_context, file_path).catch(dbg.warn.bind(this));
-                await nb_native().fs.unlink(fs_context, file_path);
-                if (stat) {
-                    await this.append_to_reclaim_wal(fs_context, file_path, stat);
-                }
-            } catch (err) {
+            try {
+                let stat;
+                if (config.NSFS_GLACIER_DMAPI_ENABLE_TAPE_RECLAIM) {
+                    try {
+                        stat = await nb_native().fs.stat(fs_context, file_path);
+                    } catch (e) {
+                        if (!['ENOENT', 'EISDIR'].includes(e.code)) dbg.warn('delete_object stat failed', file_path, e);
+                    }
+                }
+                await nb_native().fs.unlink(fs_context, file_path);
+                if (stat?.xattr?.[Glacier.GPFS_DMAPI_XATTR_TAPE_INDICATOR]) {
+                    await this.append_to_reclaim_wal(fs_context, file_path, stat).catch(dbg.warn.bind(this));
+                }
+            } catch (err) {
                 if (err.code !== 'ENOENT' && err.code !== 'EISDIR') throw err;
             }

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

src/sdk/glacier_tapecloud.js (2)

471-484: Unused parameter: failure_recorder in reclaim

failure_recorder is accepted but never used. Either wire a failure recorder path (if partial failures will be tracked) or drop the param for clarity. The former is preferable if retry semantics are needed.

---

196-207: Don’t return success on reclaim failures — keep WAL for retry

TapeCloudUtils.reclaim() always returns true, even when exec() throws. This will delete WAL files and lose entries.

Apply this fix to propagate failure:

-    static async reclaim(file) {
+    static async reclaim(file) {
         try {
             dbg.log1("Starting reclaim for file", file);
             const out = await exec(`${get_bin_path(TapeCloudUtils.RECLAIM_SCRIPT)} ${file}`, { return_stdout: true });
             dbg.log4("reclaim finished with:", out);
             dbg.log1("Finished reclaim for file", file);
-        } catch (error) {
-            dbg.error("Failed to run TapeCloudUtils.reclaim for file:", file, "due to error:", error);
-        }
-
-        return true;
+            return true;
+        } catch (error) {
+            dbg.error("Failed to run TapeCloudUtils.reclaim for file:", file, "due to error:", error);
+            return false;
+        }
     }

src/sdk/glacier.js (2)

75-84: Tidy IBMUID JSDoc phrasing

Clarify wording.

-    /**
-     * GPFS_DMAPI_XATTR_TAPE_UID xattr contains UID which contains the unique ID of the UID
+    /**
+     * GPFS_DMAPI_XATTR_TAPE_UID xattr contains the unique ID of the file

---

197-209: Fix JSDoc typo in reclaim description

"inindexed" → "unindexed".

-    /**
-     * reclaim cleans up inindexed items in the underlying
+    /**
+     * reclaim cleans up unindexed items in the underlying

src/sdk/namespace_fs.js (2)

1410-1411: Consider explicit ENOENT handling instead of silently swallowing all errors.

The .catch(_.noop) suppresses all stat errors (EPERM, EIO, etc.), not just ENOENT when the destination doesn't exist yet. While this might not break functionality, it could hide unexpected issues.

Consider applying the suggested fix from the past review comment:

-const file_path_stat = config.NSFS_GLACIER_DMAPI_ENABLE_TAPE_RECLAIM &&
-    await nb_native().fs.stat(fs_context, file_path).catch(_.noop);
+let file_path_stat;
+if (config.NSFS_GLACIER_DMAPI_ENABLE_TAPE_RECLAIM) {
+    try {
+        file_path_stat = await nb_native().fs.stat(fs_context, file_path);
+    } catch (err) {
+        if (err.code !== 'ENOENT') {
+            dbg.warn('append_to_reclaim_wal: failed to stat destination', file_path, err);
+        }
+        // Destination doesn't exist yet or stat failed, no reclaim needed
+    }
+}

---

3733-3753: Add error handling to prevent reclaim logging failures from breaking operations.

The past review comment requested wrapping this method's body in try-catch so failures don't bubble up and break the main operation flow. This has not been implemented yet.

As suggested in the previous review, apply this error handling:

 async append_to_reclaim_wal(fs_context, file_path, stat) {
     if (!config.NSFS_GLACIER_LOGS_ENABLED || !config.NSFS_GLACIER_DMAPI_ENABLE_TAPE_RECLAIM) return;

-    if (!stat) {
-        stat = await nb_native().fs.stat(fs_context, file_path);
-    }
-
-    const data = JSON.stringify({
-        full_path: file_path,
-        logical_size: stat.size,
-        ea: stat.xattr,
-    });
-    await NamespaceFS.reclaim_wal.append(data);
+    try {
+        if (!stat) {
+            stat = await nb_native().fs.stat(fs_context, file_path);
+        }
+
+        const data = JSON.stringify({
+            full_path: file_path,
+            logical_size: stat.size,
+            ea: stat.xattr,
+        });
+        await NamespaceFS.reclaim_wal.append(data);
+    } catch (err) {
+        // Log error but don't fail the operation
+        dbg.error('append_to_reclaim_wal: failed to log reclaim entry', file_path, err);
+    }
 }

Based on learnings

src/test/unit_tests/nsfs/test_nsfs_glacier_backend.js (1)

171-179: Close reclaim WAL in teardown to avoid FD leak

NamespaceFS._reclaim_wal is opened but not closed in mocha.after(). Add close similar to migrate/restore to avoid dangling handles.

 mocha.after(async function() {
-        await Promise.all([
-            fs.rm(ns_src_bucket_path, { recursive: true, force: true }),
-            fs.rm(config.NSFS_GLACIER_LOGS_DIR, { recursive: true, force: true }),
-        ]);
+        try {
+            await NamespaceFS._reclaim_wal?.close();
+        } finally {
+            await Promise.all([
+                fs.rm(ns_src_bucket_path, { recursive: true, force: true }),
+                fs.rm(config.NSFS_GLACIER_LOGS_DIR, { recursive: true, force: true }),
+            ]);
+        }
     });

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro